-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Version regex to support ranges #359
Conversation
Codecov Report
@@ Coverage Diff @@
## main #359 +/- ##
==========================================
+ Coverage 81.46% 82.11% +0.64%
==========================================
Files 21 21
Lines 928 928
==========================================
+ Hits 756 762 +6
+ Misses 119 116 -3
+ Partials 53 50 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -69,7 +77,51 @@ var _ = Describe("Operator Spec Validations", func() { | |||
})) | |||
|
|||
Expect(err).To(HaveOccurred(), "expected error for invalid semver %q", invalidSemver) | |||
Expect(err.Error()).To(ContainSubstring("spec.version in body should match '^(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)(-(0|[1-9]\\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*)(\\.(0|[1-9]\\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*))*)?(\\+([0-9a-zA-Z-]+(\\.[0-9a-zA-Z-]+)*))?$'")) | |||
Expect(err.Error()).To(ContainSubstring("spec.version in body should match '^(\\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\\^)\\s*(v?(0|[1-9]\\d*|[x|X|\\*])(\\.(0|[1-9]\\d*|x|X|\\*]))?(\\.(0|[1-9]\\d*|x|X|\\*))?(-([0-9A-Za-z\\-]+(\\.[0-9A-Za-z\\-]+)*))?(\\+([0-9A-Za-z\\-]+(\\.[0-9A-Za-z\\-]+)*))?)\\s*)((?:\\s+|,\\s*|\\s*\\|\\|\\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\\^)\\s*(v?(0|[1-9]\\d*|x|X|\\*])(\\.(0|[1-9]\\d*|x|X|\\*))?(\\.(0|[1-9]\\d*|x|X|\\*]))?(-([0-9A-Za-z\\-]+(\\.[0-9A-Za-z\\-]+)*))?(\\+([0-9A-Za-z\\-]+(\\.[0-9A-Za-z\\-]+)*))?)\\s*)*$'")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we get anything out of including the entire regexp pattern in the assertion? I wonder if we should just check for substring spec.version in body should match
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not, there's only one important regex per field; it's the field name that's important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits. But lgtm!
@joelanford made requested changes. |
Fixes operator-framework#345 Add positive and negative test cases. Signed-off-by: Todd Short <tshort@redhat.com>
LGTM However, do we need to keep this PR held open until the backing implementation described in #346 is implemented? I'm guessing if we merge without masterminds in the picture, the result is that masterminds-but-not-blang ranges will result in a reconcile error instead of a validation error? |
Yes, but it's necessary to be able to test ("get to") the reconcile code. |
@joelanford since there's a PR up for the MM code changes (updates), do you have any objections to this being merged? |
Closing in favor of #374 (which includes this). |
Fixes #345
Add positive and negative test cases.
Description
Reviewer Checklist